Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(slider): support mouse & touch all the time #2327

Merged
merged 5 commits into from
Apr 18, 2022

Conversation

mhthies
Copy link
Contributor

@mhthies mhthies commented Apr 17, 2022

Description

As proposed in #1988, I did a rework of the touch event handling of the slider component. The new touch event code

  • does not prevent mouse input (fixes [slider] cannot move slider using mouse on iPad #1988)
  • tracks Touch identifier to avoid confusion by other fingers on the touch screen
  • handles touchcancel events in a sensible way
  • does no dynamic binding and unbinding to all touch events on the page. In combination with the second point, this enables multitouch sliding of multiple sliders on the same page (I know, nobody asked for this, but it works. ;) ) – Unfortunately, multitouch sliding of the first and second thumb of the same slider is not easy to implement due global state within each slider module.
  • only binds to touchstart events on the .thumb element to avoid accidential sliding while scrolling the page on a mobile device (obsoletes [slider] Add option to disable touch-/mouse-sliding on the track #1987)

The last point can be considered a disadvantage/regression compared to the old code, if you consider touch-sliding anywhere on the slider as a feature. However, in my experience, it's really annoying to change a slider position (which has immediate real-world effects in my home automation software) when scrolling the page and accidentially hitting a slider. Using the mouse, one can still slide the slider anywhere on its track.

Testcase

https://jsfiddle.net/jyg4wtcq/
https://fomantic-ui.com/jsfiddle/#!/jyg4wtcq (for mobile devices)

Screenshot (if possible)

--

Closes

This fixes fomantic#1988 (mouse interaction on touch devices) but disables
dragging the slider via touch input. Touch-tapping for slider
positioning still works through browsers' mouse event emulation.
Touch-dragging will be re-added in a modified form in a later commit.
In contrast to the old touch event handling, this code
- handles touchcancel events in a sensible way
- tracks Touch identifier to avoid confusion by other fingers on the
  touch screen
- does not interfere with mouse sliding
- binds only to .thumb elements to avoid accidential sliding while
  scrolling the page on a mobile device
@lubber-de lubber-de added lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews device/mobile Any issues relating to mobile devices labels Apr 17, 2022
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very well done PR so far 👍🏼
Thank you!

I only found a typo and have a question. Please see my comments.

A little additional request when done/answered:
Please provide two links for jsfiddle testing for this:

Because I would like @jamessampford to test this on his ipad/mouse setup .
I believe you already tested it so far on your Android/bt mouse setup (?)

I have to rely/trust on your experience here, because i can only use Browserstack to emulate a wider range of mobile devices but none of those have a real mouse attached to the devices, so the only situation i can personally test is if the changes basically still work on "usual" setups.

src/definitions/modules/slider.js Outdated Show resolved Hide resolved
src/definitions/modules/slider.js Show resolved Hide resolved
@lubber-de lubber-de added this to the 2.9.0 milestone Apr 18, 2022
@jamessampford
Copy link
Contributor

@lubber-de This seems to work (both dragging and clicking) on my iPad (iPadOS 15.4.1, latest version) using:

  • just my finger
  • with Apple Pencil
  • using the trackpad on my keyboard (not the Magic Keyboard, not Bluetooth connected)

Also works on my Android phone still but I can only try with finger

It may be a good idea to test in Windows with a touchscreen in all browsers, as well as hoping that someone else has a trackpad keyboard and/or Bluetooth mouse to verify in iOS/iPadOS

As this also relates to #1989 with the dropdown module, the same improvement will also need to be implemented there (and anywhere else similar)

Co-authored-by: Marco 'Lubber' Wienkoop <c64@lubber.de>
@mhthies
Copy link
Contributor Author

mhthies commented Apr 18, 2022

I believe you already tested it so far on your Android/bt mouse setup (?)

Yes, I tested with desktop Chrome on Linux (touch + mouse), mobile Chrome on Android (touch + mouse) and mobile Firefox on Android (touch + mouse).

It may be a good idea to test in Windows with a touchscreen in all browsers, as well as hoping that someone else has a trackpad keyboard and/or Bluetooth mouse to verify in iOS/iPadOS

This is a good idea, although I'm hoping that all modern desktop browsers stick to the Touch API as it is described on MDN. Unfortunately, I don't have Windows touch device at hand.

As this also relates to #1989 with the dropdown module, the same improvement will also need to be implemented there (and anywhere else similar)

Yes, I'm already working on this one. Another PR will follow for this, as soon as I can fix some strange behaviour in touch interaction of nested dropdown menus (which is already present in the current code).

This should avoid unwanted interference with touch-enabled parent
elements. It does not seem to have unwanted side-effects on the slider
itself.
@mhthies
Copy link
Contributor Author

mhthies commented Apr 18, 2022

Here is the updated JSFiddle:

@lubber-de
Copy link
Member

@jamessampford could you please try/approve the new jsfiddle on your setup again ? Thx

@jamessampford
Copy link
Contributor

@lubber-de Seems to still work the same with the same 3 different ways of testing it as before (with dragging and clicking/tapping) with both the sliders

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍🏼

@lubber-de lubber-de changed the title [slider] Rework touch input fix(slider): support mouse & touch all the time Apr 18, 2022
@lubber-de lubber-de merged commit f964821 into fomantic:develop Apr 18, 2022
@lubber-de
Copy link
Member

@all-contributors please add @mhthies for code

@allcontributors
Copy link
Contributor

@lubber-de

I've put up a pull request to add @mhthies! 🎉

@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Apr 18, 2022
lubber-de pushed a commit that referenced this pull request May 1, 2022
This PR fixes #1989 in a similar way as my previous PR for the slider (#2327): First, I removed all special event handling for touch events and the whole distiction between touch- and non-touch devices, so mouse events will be handled on touch devices as well. Then, I added only a few touch event bindings for the things that don't work via mouse event emulation of the mobile web browsers.

In addition, I fixed two minor bugs with nested submenus being not correctly hidden in some cases (see commit messages for more details).

I tested everything on

Chrome mobile on Android (with touch and Bluetooth mouse)
Firefox mobile on Android (with touch and Bluetooth mouse)
Chromium on Linux/X11 (with multitouch screen and mouse)
Firefox on Linux/X11 (with multitouch screen and mouse)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
device/mobile Any issues relating to mobile devices lang/javascript Anything involving JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[slider] cannot move slider using mouse on iPad
3 participants